Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce "Target" as an alias of "Range" #1409

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

zickgraf
Copy link
Member

@zickgraf zickgraf commented Aug 2, 2023

Do not declare it as a synonym to avoid conflicts with other GAP packages.

"Target" seems to be used more widely and has typographical advantages:

  1. "Target" naturally comes after "Source" when sorting alphabetically.
  2. "Source" and "Target" have the same number of letters.
  3. It better matches the maps "s" and "t" which are sometimes used to define categories.

@mohamed-barakat I have already talked to @sebastianpos and @kamalsaleh about this and both are in favor of this change. Do you also approve of this idea?

Do not declare it as a synonym to avoid conflicts with other GAP packages.

"Target" seems to be used more widely and has typographical advantages:
1. "Target" naturally comes after "Source" when sorting alphabetically.
2. "Source" and "Target" have the same number of letters.
3. It better matches the maps "s" and "t" which are sometimes used to
   define categories.
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 81.81% and project coverage change: -0.01% ⚠️

Comparison is base (3e03580) 80.62% compared to head (f257dab) 80.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1409      +/-   ##
==========================================
- Coverage   80.62%   80.62%   -0.01%     
==========================================
  Files         492      492              
  Lines       62417    62433      +16     
==========================================
+ Hits        50323    50334      +11     
- Misses      12094    12099       +5     
Flag Coverage Δ
ActionsForCAP 64.76% <ø> (ø)
AttributeCategoryForCAP 88.91% <ø> (ø)
CAP 83.85% <100.00%> (+<0.01%) ⬆️
CartesianCategories 93.50% <ø> (ø)
CompilerForCAP 94.25% <73.33%> (-0.10%) ⬇️
ComplexesAndFilteredObjectsForCAP 73.68% <ø> (ø)
FreydCategoriesForCAP 81.25% <ø> (ø)
GeneralizedMorphismsForCAP 61.40% <ø> (ø)
GradedModulePresentationsForCAP 44.59% <ø> (ø)
GroupRepresentationsForCAP 72.06% <ø> (ø)
HomologicalAlgebraForCAP 73.21% <ø> (ø)
InternalExteriorAlgebraForCAP 93.08% <ø> (ø)
LinearAlgebraForCAP 66.57% <ø> (ø)
ModulePresentationsForCAP 68.41% <ø> (ø)
ModulesOverLocalRingsForCAP 90.70% <ø> (ø)
MonoidalCategories 89.00% <ø> (ø)
ToricSheaves 21.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
CompilerForCAP/gap/InferDataTypes.gi 70.09% <50.00%> (-0.06%) ⬇️
CompilerForCAP/gap/LogicTemplates.gi 99.06% <70.00%> (-0.94%) ⬇️
CAP/PackageInfo.g 100.00% <100.00%> (ø)
CAP/gap/CategoryMorphisms.gd 100.00% <100.00%> (ø)
CAP/gap/CategoryMorphisms.gi 74.93% <100.00%> (+0.13%) ⬆️
CAP/gap/CategoryTwoCells.gd 100.00% <100.00%> (ø)
CAP/gap/CategoryTwoCells.gi 72.72% <100.00%> (+2.72%) ⬆️
CompilerForCAP/PackageInfo.g 100.00% <100.00%> (ø)
CompilerForCAP/gap/Logic.gi 99.83% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mohamed-barakat
Copy link
Member

@mohamed-barakat I have already talked to @sebastianpos and @kamalsaleh about this and both are in favor of this change. Do you also approve of this idea?

I find this idea great and would have suggested it anyway :) Thank you very much.

@mohamed-barakat
Copy link
Member

mohamed-barakat commented Aug 2, 2023

I would even go so far and make Range an alias of Target, if this would technically be possible.

@zickgraf
Copy link
Member Author

zickgraf commented Aug 2, 2023

I find this idea great and would have suggested it anyway :) Thank you very much.

Perfect, then I will discuss this once more verbally with @sebastianpos and then merge this PR.

I would even go so far and make Range an alias of Target, if this would technically be possible.

This can certainly be a next step, but for now I want to keep things 100% compatible (and first see if this causes problems with other GAP packages). If we want to make Target the new default, we should also discuss and change some other things like ...WithGivenRange and RangeCategoryOfHomomorphismStructure.

@mohamed-barakat
Copy link
Member

Agree on all points.

@zickgraf
Copy link
Member Author

zickgraf commented Aug 3, 2023

For the record: Range was introduced in 13fe1b0, the fourth commit of CAP :D So it has really been there since the beginning.

@zickgraf zickgraf merged commit 1848df6 into homalg-project:master Aug 3, 2023
3 of 4 checks passed
@zickgraf zickgraf deleted the target branch August 3, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants